Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native Class Roadmap #338

Closed
wants to merge 2 commits into from
Closed

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Jun 15, 2018

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this! Great insight into the future of Ember.

that it fulfills a certain interface.

For instance, a Component can be any class which receives an `args` hash and
optionally implements any of the component lifecycle hooks:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non important, curious question - Would this in effect reduce lookup time for properties/non existent properties, especially for things are are high up the prototype chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory yes, though I wouldn't count on it being that huge of a win. In aggregate though, who knows 😄


If we go this direction, it means we cannot specify any base class behavior. One
issue that arises is the fact that dependency injections will not be available
to the class _during_ object construction. They can be assigned _after_ only.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this result affect initializers/instance-initializers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initializers and instance-initializers are (as I understand them) essentially pure functions that run at the beginning off app creation/instantiation. They would still receive the app definition/instance and container as such, and wouldn't be directly impacted by this change.

However, and injections which are setup by initializers would be affected. All injections are currently assigned by this mechanism, and assigning after would make them unavailable.

It's worth noting that only potential solutions mentioned in the RFC which would work for injections done this way would be the init hook and passing the injections to the constructor as named parameters. Personally, I am not a fan of the ability to unilaterally inject items onto any type thing (e.g. store automatically injected to routes/controllers) and think it would make sense to recommend users manually declare the dependency.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps in v4 we have init() to support this and in Ember v5 we drop it and make everything explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be an option if we decided to go with the other DI options and drop init altogether. It would give us a good timeline to clean things up on as well, and falls neatly inline with the proposed timeline for removing EmberObject.

@lupestro
Copy link
Contributor

Super minor text nit and personal pet peeve: “a myriad of”
“Myriad” is a number, not a collective noun. It means “ten thousand”. If swapping in “ten thousand” sounds funny, make that sound right, then go back to myriad, so “myriad things” is better usage.
(I will now retire my fussy school-marm hat for the evening. :) )
This was a great writeup. I’m glad the ball is rolling on this.

@rtablada
Copy link
Contributor

rtablada commented Jun 20, 2018

A thought on Mixin migration. I'm not 💯sure of the validity or plausibility of this, but would it make sense to have a legacy apply-mixin function for use in classes?

Here's an example I'm thinking (inspired by redux createStore)

import Route from '@ember/routing/route';
import ApplicationRouteMixin from 'ember-simple-auth/mixins/application-route-mixin';
import applyMixin from '@ember/apply-mixin';

class ApplicationRoute extends Route {
  // Stuff
};

export default applyMixin(ApplicationRoute, ApplicationRouteMixin);

Or a different API might be more like (more like redux applyMiddleware)

import Route from '@ember/routing/route';
import ApplicationRouteMixin from 'ember-simple-auth/mixins/application-route-mixin';
import createClassFromMixin from '@ember/create-class-from-mixin';

const createApplicationRoute = createClassFromMixin(ApplicationRouteMixin, ...any other mixins);

class ApplicationRoute extends Route {
  // Stuff
};

export default createApplicationRoute(ApplicationRoute);

@pzuraq
Copy link
Contributor Author

pzuraq commented Jun 20, 2018

Pre-removal of EmberObject, I'm not sure it would be necessary. We can still use class Foo extends EmberObject.extend(Mixin), so they are effectively supported. It would just add another way to do the same thing, though it may be clearer. I'm not opposed to it personally.

Post-removal of EmberObject, I don't think it should be part of Ember. It would be very difficult, if not impossible, to maximize all of the benefits of removing EmberObject and switching to native classes, and make an applyMixins which would faithfully apply the mixins and result in the exact same final class.

That said, I'm considering lots of tiny edge cases and small behaviors that are tied to Ember/Mixins. I think there's definitely room to make a library that applies mixins in mostly complete fashion, so legacy users can use mostly the same mixin definitions with relatively minor changes. I also think that there is room for a much better general mixin solution that actually leverages native classes (for inspiration, see this article that proposes one such system).

@dfreeman
Copy link
Contributor

The wider JS community (or at least a vocal subset) seems to like that 'functional mixin' pattern — the pipeline operator proposal calls out how it would make applying them nicer, and the original author of that post has a stage 1 proposal for formalizing them with dedicated syntax.

@nruth
Copy link

nruth commented Jun 25, 2018

@lupestro the myriad usage is fine going by the OED. Hoping Ember isn't classical history :)

@lupestro
Copy link
Contributor

@nruth And I hear rumors that prepositions are now accepted to end sentences with. (“...mere anarchy is loosed upon the world...”) I am finally old enough to experience the eternal rules of language changing in even one brief lifetime. (But of course they have ever done so...) NVM, carry on, carry on! 😀

```


3. **Utilize private global state to provide the container to injections during

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but then it should be done for getOwner() so that even a manual call to getOwner(this) inside the constructor will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this would make the most sense if we pursued this strategy. Will update the RFC to reflect that

@pzuraq
Copy link
Contributor Author

pzuraq commented Jul 30, 2018

I had a great conversation with @mike-north the other day about this, he pointed out a few concerns with this pattern going forward. I'm posting them here for discussion, but they should definitely be addressed by updates to the RFC at some point:

  1. Interface-based design can be very cumbersome dependending on the way it is done. Other frameworks have implemented it, in particular Angular 2+, in a form where interfaces are scoped down to very specific functionality that any class can implement. For instance, the onInit interface tells Angular that a class should fire the ngOnInit() hook:

    import { Component, OnInit } from '@angular/core';
    
    @Component({})
    export class HeroDetailComponent implements OnInit {
      ngOnInit() {
      }
    }

    This can be very confusing, because apparently without the OnInit interface being used ngOnInit will not fire at all (part of Angular's inferencing of type information). It can also become combinatorially complex, as every lifecycle hook must become an interface, resulting in massive unparseable component definitions.

    When writing this proposal, I envisioned interfaces representing a full type, like they are today. Typescript users would actually end up with very similar code to what exists currently:

    import { Component } from '@ember/interfaces';
    
    export default class HeroDetailComponent implements Component {
      didInsertElement() {
    
      }
    }

    And native Javascript users would not extend a class at all. This would keep definitions of interfaces and types minimal and not lead to combinatorial complexity, though it could be less flexible than the alternative.

  2. If base classes are not used, then there is no way to specify the placement of the call to super for a hook. This means that users cannot easily execute code both before and after a hook, which is less flexible:

    export default class MyComponent extends Component {
      didInsertElement() {
        // do something before
    
        super.didInsertElement();
    
        // do something after
      }
    }

    This can be mostly mitigated with "bookend" hooks, such as willInsertElement/didInsertElement, willDestroy/didDestroy, etc. It should also be noted that users can still maintain their own class hierarchy's and control calls to super in those, so this would only be a concern with framework code.

  3. If base classes are not used, there is no easy way to intercept arguments to a hook, or the return value of a hook. The effects here have to be studied more closely, as there are times when arguments should generally not be used or modified, and times where it may make sense to modify them. It may also be possible to modify arguments in bookend hooks, though that would come with its own set of problems and concerns.

@tomdale
Copy link
Member

tomdale commented Dec 4, 2018

Removing the Octane label from this, as this is a more future-looking RFC that needs to be updated to reflect the current state of the world, and is unlikely to happen in the Octane timeframe.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 26, 2019

This RFC is pretty out if date at this point. Closing it for now, and will reopen sometime post Octane when we’ve had time to update it.

@pzuraq pzuraq closed this Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants